Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

DHT API #36

Merged
merged 8 commits into from
Nov 8, 2016
Merged

DHT API #36

merged 8 commits into from
Nov 8, 2016

Conversation

daviddias
Copy link
Contributor

@daviddias daviddias commented Jun 28, 2016

WIP

@victorb
Copy link
Contributor

victorb commented Sep 17, 2016

Build fails because I've set the npm test to fail instead of invalid command. Merging #74 will make us a have a valid CI for CircleCI

@daviddias
Copy link
Contributor Author

@victorbjelkholm do you have a branch of these tests running on js-ipfs-api?

after((done) => {
common.teardown(done)
})
xdescribe('.findpeer', () => {})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: xdrescribe)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending test ;)

@daviddias
Copy link
Contributor Author

daviddias commented Sep 18, 2016

The tests need to be updated to follow the same pattern of the remaining tests in the interface-ipfs-core, the pattern is:

  • have a section for "callback API" and heavily test that one
  • be creative about tests, if you can make them faster or find bugs in the impl through then, big 👍
  • have a section for "promise API" and test at least each function once

Remember that once these things get merged, then it becomes official docs for everyone that consumes these APIs, it is very important to be very careful.

@victorb
Copy link
Contributor

victorb commented Sep 18, 2016

@diasdavid

@victorbjelkholm do you have a branch of these tests running on js-ipfs-api?

Yeah, over here: ipfs-inactive/js-ipfs-http-client#385

have a section for "callback API" and heavily test that one

👍

be creative about tests, if you can make them faster or find bugs in the impl through then, big 👍

👍

have a section for "promise API" and test at least each function once

Hm, not so sure about this one. The promise API should be tested at one place, to make sure promisify is working correctly, but I don't think we need to test every single function in both the callback way and promise. Makes more sense to have a couple of tests to make sure that one way is working, then always stick with the other otherwise.

@daviddias
Copy link
Contributor Author

having a test for each method (not all combinations, just a test for each method) has detected in the past missing 'promisify' wraps.

@daviddias
Copy link
Contributor Author

@victorbjelkholm I believe you are still on top of this, correct?

@victorb
Copy link
Contributor

victorb commented Nov 8, 2016

@diasdavid Yessir!

@victorb
Copy link
Contributor

victorb commented Nov 8, 2016

@diasdavid tests are passing, should be ready to be merged so we can move on with ipfs-inactive/js-ipfs-http-client#385

@daviddias daviddias merged commit 50b614a into master Nov 8, 2016
@daviddias daviddias deleted the dht-api branch November 8, 2016 17:49
@daviddias
Copy link
Contributor Author

awesome :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants